Skip to content

refactor(daemon): consolidate session-script test factories and drop saveScriptDefaultedHealedPath - #1508

Merged
thymikee merged 1 commit into
mainfrom
p4a/prep-test-factories
Jul 31, 2026
Merged

refactor(daemon): consolidate session-script test factories and drop saveScriptDefaultedHealedPath#1508
thymikee merged 1 commit into
mainfrom
p4a/prep-test-factories

Conversation

@thymikee

Copy link
Copy Markdown
Member

Preparatory slice for P4a (#1478). This introduces no aggregate. No ReplaySessionTransaction, no SessionScriptPublication, no tagged session-script cluster, and no production publication writer is migrated — all of that is the separate P4a PR. This exists so that PR is reviewable.

1. Session-script test factories

~250 direct writes of saveScript*/recordSession were spread across the daemon test tree as inline object literals. P4a proper has to change the shape of every one of them, so they get a name first.

Three factories next to the existing makeIosSession in src/__tests__/test-utils/session-factories.ts:

factory state
makeAuthoringSession ADR 0016 ordinary authoring recording — recordSession, no repair boundary
makeRepairArmedSession ADR 0012 decision 6 ARMED — plus saveScriptBoundary, the flag the writer's repairArmed keys off
makeRepairCompleteSession ARMED -> COMPLETE — committable

40 inline session literals across 11 test files now go through them. The heaviest is session-script-writer.test.ts (24 literals, 20+ of which spelled out the same three-to-five-field combination). Named "authoring" rather than "recording" because session.recording is the unrelated screen-capture state, and daemon-runtime-recording-teardown.test.ts already has a local makeRecordingSession meaning that.

The factories write today's fields — pure refactor, no production change, no behavior change, no assertion weakened.

Deliberately left explicit

  • session-action-recorder.test.ts / selector-recording.test.ts (9 sites) set saveScriptBoundary: 0 with no recordSession. That is the repair-armed marker in isolation, which is exactly the predicate those tests exercise; forcing them through a factory would add a field production couples but these tests deliberately don't.
  • request-router-repair-expired.test.ts's tombstonedSession — a boundary plus repairSourcePath with no recording, one site, no repetition to remove.
  • session-store.test.ts keeps its own makeSession: one test mutates session.device.name, and the shared factories hand back the module-level IOS_SIMULATOR by reference, so routing it through them would make that mutation leak across tests. Only the dead-field lines were removed there.
  • request-router-typed-error.test.ts keeps its tenant-scoped simulator (simulatorSetPath), now passed into the shared factories as TENANT_SESSION_DEFAULTS rather than re-spelled per test.
  • Assertion-only references (session-replay-repair-loop/-empty-tail/-acceptance, the two integration scenarios, request-save-script-transports.test.ts) were left alone — they read the fields, they don't construct them.

2. saveScriptDefaultedHealedPath deleted

Zero production readers, verified. A repo-wide grep finds only three writers (session-replay-runtime.ts:867/870, session-action-recorder.ts:83), the type declaration, the R7 ownership row, and one test assertion. Nothing branches on it: the writer's refuse-on-exist guard (publishHealedScriptAtomically) has been uniform since #1235 — an explicit --save-script=<path> and the default .healed.ad sibling are refused identically — so the flag has been bookkeeping with no consumer.

Deleting it required, in the same commit:

  • dropping its row from SESSION_STATE_FIELD_OWNERS in scripts/layering/session-state.ts;
  • lowering the R10 baseline in scripts/layering/daemon-modularity.ts: writerOwnedFields 30 -> 29, ownerFileClaims 42 -> 40 (the field claimed two owner files). The ratchet fails on a drop as well as a growth, so this could not be deferred.

scripts/layering/daemon-modularity.test.ts needed no change — it asserts against DAEMON_MODULARITY_BASELINE dynamically rather than against literals. scripts/layering/model.test.ts was checked: its AST fixture string uses saveScriptComplete, not the deleted field, and its field-count assertions derive from the tables, so it is unaffected.

The one test that asserted on the flag (session-script-writer.test.ts) kept its other concern — that close --save-script=<explicit path> re-points a defaulted-healed repair and publishes cleanly to the new target — and lost only the marker assertion.

Gates

pnpm check:layering
Layering guard: OK — ... all 40 SessionState fields are classified and every write is
inside its declared owner (R7); ... R10 pins R7 at 29 writer-owned fields / 40 owner
claims, ...

pnpm check:affected --base origin/main --run: format, lint, typecheck, layering, fallow, mcp-metadata and build all pass. vitest-related runs 236 files / 2269 tests with 1 pre-existing failure: src/daemon/handlers/__tests__/session-test-runner.test.ts ("test dispatches per-platform scripts", ['ios','android'] vs ['android','ios']). It reproduces unchanged on origin/main (32b9db2d7) with this branch stashed, is in the P3 worker's session-test*.ts territory, and is pulled into the related set only because this PR touches src/daemon/types.ts. Every test this PR actually edits passes.

Coordination

The only shared-file edit is the two-line writerOwnedFields/ownerFileClaims baseline in scripts/layering/daemon-modularity.ts — localized, well away from the R10 logical-module policies the P3 worker is editing. session-replay-runtime.ts is touched only at ~867 (the arming step), disjoint from P3's ~461.

Refs #1478

🤖 Generated with Claude Code

https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8


Generated by Claude Code

…saveScriptDefaultedHealedPath

Preparatory slice for P4a (#1478). No aggregate, no transaction type, no
publication-writer migration — those land separately.

Two things:

1. Name the session-script session states in the shared test factories
   (`makeAuthoringSession`, `makeRepairArmedSession`,
   `makeRepairCompleteSession`) and route 40 inline session literals across
   11 test files through them. The `saveScript*` fields are not independent
   — recording without a boundary is ordinary authoring, a boundary without
   `saveScriptComplete` is an ARMED-but-uncommittable repair, and only the
   COMPLETE combination publishes — so re-deriving the combination per test
   buried the distinction each test was actually about. Pure refactor: the
   factories write today's fields and no assertion was weakened.

2. Delete `saveScriptDefaultedHealedPath`. It had zero production readers:
   the writer's refuse-on-exist guard has been uniform since #1235, so the
   flag was written in three places and never consulted. Removing it takes
   its R7 owner row with it and lowers the R10 baseline from 30/42 to 29/40
   (the ratchet fails on a drop too, so this cannot be deferred).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RXQLYV7etZx3gcXsUsrQJ8
@github-actions

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.91 MB 1.91 MB -109 B
JS gzip 611.8 kB 611.7 kB -35 B
npm tarball 729.4 kB 729.4 kB -33 B
npm unpacked 2.56 MB 2.55 MB -109 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.7 ms 27.8 ms -1.0 ms
CLI --help 59.4 ms 58.9 ms -0.5 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/screenshot-result.js -37 B -18 B
dist/src/session.js -72 B -17 B

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head f1be82ebaf1e3f07a9b243441e29656767f11b75 against #1478 P4a and ADRs 0012/0016. No blocking findings.

What I verified:

  • This is genuinely preparatory only: no aggregate, publication writer migration, or production state-machine redesign is smuggled into the slice.
  • saveScriptDefaultedHealedPath has no production reader. Its three writes only tracked whether saveScriptPath came from the healed-sibling default, while publication collision/force behavior is uniformly driven by the target path and writer policy. Removing the type field, both writers, R7 ownership row, and the one marker assertion together is behavior-neutral.
  • makeAuthoringSession, makeRepairArmedSession, and makeRepairCompleteSession reproduce today’s real combinations (recordSession; then boundary; then completion) and make the ARMED/COMPLETE distinction visible. Tests that intentionally exercise malformed/marker-only states remain explicit, and the mutable-device test remains outside the shared factories.
  • Converted tests retain their original behavioral assertions; the writer suite becomes materially easier to audit without hiding target paths, action history, force state, or terminal state in the factory.
  • R7/R10 move monotonically and consistently: 40 SessionState fields remain fully classified; writer-owned fields drop 30 → 29 and owner claims 42 → 40.

Local exact-head checks:

  • focused writer/publication/replay/router set: 4 files, 59 tests passed
  • pnpm check:layering: green, including R7 at 29/40 and R9 at 76
  • git diff --check: clean

GitHub checks are green so far, including integration, layering, Fallow, FreeRange, packaged CLI, typecheck, and platform-independent smokes. Coverage plus Android/iOS smoke/release jobs were still pending at review time, so merge readiness is conditional on those exact-head jobs finishing green.

One scope clarification for the landing record: despite the P4a terminology, this PR is the test-factory/dead-field prep slice, not P4a’s aggregate or ownership migration itself. The body already states that clearly.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed exact head f1be82ebaf1e3f07a9b243441e29656767f11b75 against #1478 and ADRs 0012/0016. No findings.

saveScriptDefaultedHealedPath has no production reader; its removal preserves publication collision/force behavior, which is uniformly decided from the target path and writer policy. The new authoring, repair-armed, and repair-complete factories reproduce the current state combinations without masking intentional malformed-state tests.

Exact-head CI is green, including integration, coverage, layering, typecheck, platform smokes, and native releases. This is ready to merge as the stated P4a prep slice (not the P4a ownership/state-machine migration itself).

Residual risk: no separately authorized cross-vendor review was performed.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Jul 30, 2026
@thymikee
thymikee merged commit 1fc9169 into main Jul 31, 2026
30 checks passed
@thymikee
thymikee deleted the p4a/prep-test-factories branch July 31, 2026 05:24
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-31 05:25 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants